Introduce verification-doc component with 4 states by sarupbanskota · Pull Request #892 · code-corps/code-corps-ember

sarupbanskota · 2016-12-17T21:17:03Z

Progress on: #885

In the identity-document-file-upload component we'll:

set the upload URL to https://uploads.stripe.com/v1/files
change paramName to file
add the purpose param with value of "identity_document"
add a Stripe-Account header to accept the stripeConnectAccount.idFromStripe
add a Basic auth header to have a username of our ENV.stripe.publishableKey with no password – this probably needs to be base64 encoded (e.g. pk_test_123: would be encoded)

Ideally we should:

validate the file size is less than 8MB
validate it is either a JPEG or PNG

When the upload is finished, we'll need to:

send the id value found in the response in an action to the verification-document component
the verification-document component will then call an action to set the identityDocumentId field on the StripeConnectAccount model

joshsmith

Couple quick comments, but otherwise looking good!

joshsmith · 2016-12-17T21:18:34Z

+  we need you to upload a scan of your ID (form + submit)
+{{else if (eq stripleConnectAccount.verification_document_status 'verifying')}}
+  Please be patient while we review the document you provided.
+{{else if (eq stripleConnectAccount.verification_document_status 'verified')}}


Just a small typo here stripleConnectAccount

I would just do if (eq stripleConnectAccount.verification_document_status 'required') and if (eq stripleConnectAccount.verification_document_status 'verifying') without the branching. Otherwise, it will by default render nothing.

mm, didn't quite understand; why would it render nothing @joshsmith?

@sarupbanskota I'm saying we only want to handle the two cases of required and verifying. No need to branch then with your conditionals. This component renders something only in those two cases.

joshsmith · 2016-12-17T21:19:12Z

+    {{/payments/funds-recipient/verification-document}}
+  `);
+
+  assert.equal(this.$().text().trim(), 'template block text');


You'll need to introduce a new page object for these tests, when you get there.

sure, could you point me to an example on another test file?

Sure, the bank-account-test which is a sister component is a good example.

begedin · 2016-12-18T11:10:21Z

@sarupbanskota Good job up to this point. I'll take over and try to wrap up before you're back.

begedin · 2016-12-18T14:46:22Z

This checks all the boxes now, but there are some notes that I would like to add:

We only have some basic tests that test bindings and various if switches in the templates. We need to figure out how to test uploads themselves, and handling of events triggered by the ember-uploader component
The whole thing needs styling (all the way up to the funds-recipient component.
Identifying is not done via an authorization header. Instead, it's done by adding a key field to the form data being sent by the upload, which is set to the publishable key. Is there documentation available indicating that a basic authorization header would be preferable? I Couldn't find anything.

Either way, the process here does work. It will upload the file to stripe, then pass out the action with a stripe id parameter all the way to the route, where the route will assign it to the connect account and try to save that connect account.

This fails right now, because the API has no such endpoint, but all the failure points that I could think of are handled in a basic way - by rendering a friendly error. The errors rendered are:

file validation fails
upload fails
assigning file to account fails

joshsmith

I'm pretty okay not testing the internals of the addon. What's here looks generally good enough to me!

joshsmith · 2016-12-18T17:04:04Z

+   */
+  additionalUploadData: {
+    // required to authorize the upload request
+    key: ENV.stripe.publishableKey,


This actually works? I only saw the basic auth header in the docs.

I couldn't find that part at all. Only found this one.

begedin · 2016-12-18T17:16:16Z

It's not just that we aren't testing internals. The addon doesn't allow much in appearance stuff, so we have to basically wrap it up into an external addon, which handles the event hooks. That external addon would then be the one that displays progress/errors/etc., so we ought to test that one to ensure all of the features work. I'm not sure how, though.

In an integration test, we would have to mock a web request (to upload), which sends a progress report, as well as mocking file selection.

In a unit test, those things could be mocked by calling the handlers manually, but then we aren't testing the whole surface area.

That being said, I think it should be mergeable as is, but we need to create an issue to figure that out later.

sarupbanskota

Learned a bunch of things just checking out your progress on this PR 😍

Thank you! I'll PM you with some general questions I have

joshsmith · 2016-12-19T03:44:55Z

Haven't had a chance to code review but there are some conflicts that need resolving now. Would prefer keeping the account => stripeConnectAccount changes that were made.

joshsmith · 2016-12-19T03:45:30Z

I might be able to fix these changes when I get home this evening.

begedin · 2016-12-19T14:39:11Z

This should be good to go now.

joshsmith

Just minor things would change, otherwise looks good.

joshsmith · 2016-12-20T07:42:37Z

+    }
+  },
+
+  _validate({ type, size }) {


Would alpha order these.

joshsmith · 2016-12-20T07:43:17Z

+  _handleIdentityVerificationDocumentError() {
+    let friendlyError = new FriendlyError(VERIFICATION_DOCUMENT_ERROR);
+    return RSVP.reject(friendlyError);
+  },


Would also alpha-order here.

sarupbanskota force-pushed the feature/verification-doc branch from c33dba4 to a7d714f Compare December 17, 2016 21:17

joshsmith reviewed Dec 17, 2016

View reviewed changes

joshsmith assigned sarupbanskota Dec 17, 2016

joshsmith added the in progress label Dec 17, 2016

joshsmith added this to the Launch Donations milestone Dec 17, 2016

sarupbanskota force-pushed the feature/verification-doc branch from a7d714f to 67238dd Compare December 17, 2016 21:26

joshsmith mentioned this pull request Dec 18, 2016

Create a payments/funds-recipient/verification-document component #885

Closed

begedin assigned begedin and unassigned sarupbanskota Dec 18, 2016

begedin added needs code review and removed in progress labels Dec 18, 2016

begedin requested a review from joshsmith December 18, 2016 14:46

joshsmith approved these changes Dec 18, 2016

View reviewed changes

sarupbanskota commented Dec 18, 2016

View reviewed changes

begedin force-pushed the feature/verification-doc branch 3 times, most recently from 0cdee65 to a8aaa88 Compare December 19, 2016 14:38

joshsmith force-pushed the feature/verification-doc branch from a8aaa88 to bef0e0a Compare December 20, 2016 01:17

joshsmith reviewed Dec 20, 2016

View reviewed changes

joshsmith added needs submitter response ready to merge and removed needs code review labels Dec 20, 2016

Introduce verification-document component to payments page

8533b4b

begedin force-pushed the feature/verification-doc branch from bef0e0a to 8533b4b Compare December 20, 2016 11:15

begedin merged commit f8ad35c into develop Dec 20, 2016

begedin deleted the feature/verification-doc branch December 20, 2016 11:20

Uh oh!

Conversation

sarupbanskota commented Dec 17, 2016 • edited by begedin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin commented Dec 18, 2016

Uh oh!

begedin commented Dec 18, 2016

Uh oh!

joshsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

begedin commented Dec 18, 2016

Uh oh!

sarupbanskota left a comment

Choose a reason for hiding this comment

Uh oh!

joshsmith commented Dec 19, 2016

Uh oh!

joshsmith commented Dec 19, 2016

Uh oh!

begedin commented Dec 19, 2016

Uh oh!

joshsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sarupbanskota commented Dec 17, 2016 •

edited by begedin

Loading